-
-
Notifications
You must be signed in to change notification settings - Fork 218
config: Android: 16 KB page size #3565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @ajaysehwal, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Android application's compatibility and stability, particularly for devices utilizing 16KB page sizes. It streamlines the build process by adjusting native library handling and introduces protective measures for third-party modules that rely on native code, preventing potential crashes. Furthermore, a new diagnostic tool is included to assist in verifying the alignment of native libraries, reinforcing the project's commitment to robust Android development practices. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively addresses the requirement for 16KB page-size compliance on Android. The changes to the Gradle configuration, Android Manifest, and NDK version are all appropriate and well-justified with comments. The strategy of patching dependencies like react-native-tor and react-native-vision-camera to gracefully handle potential native library loading failures is excellent, as it enhances the app's resilience. The inclusion of the check_elf_alignment.sh script is a commendable addition for verifying the correctness of these changes. My review includes one suggestion to further improve the robustness of this new verification script.
| @@ -0,0 +1,113 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this script more robust, I recommend adding set -euo pipefail at the beginning.
set -e: Exits the script immediately if any command fails. This would prevent the script from providing a misleading 'success' message if, for example, theunzipcommand at line 64 fails.set -u: Treats unset variables as an error, which helps catch typos in variable names.set -o pipefail: Ensures that a pipeline's exit code is the exit code of the rightmost command to exit with a non-zero status, or zero if all commands of the pipeline exit successfully.
This is a standard best practice for writing safer shell scripts.
| #!/bin/bash | |
| #!/bin/bash | |
| set -euo pipefail |
56bdeda to
6cd6e95
Compare
6cd6e95 to
2066252
Compare
2066252 to
03e6fc4
Compare
Description
Relates to issue: ZEUS-3293
This pull request is categorized as a:
Checklist
yarn run tscand made sure my code compiles correctlyyarn run lintand made sure my code didn’t contain any problematic patternsyarn run prettierand made sure my code is formatted correctlyyarn run testand made sure all of the tests passTesting
If you modified or added a utility file, did you add new unit tests?
I have tested this PR on the following platforms (please specify OS version and phone model/VM):
I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):
Locales
Third Party Dependencies and Packages
yarnafter this PR is merged inpackage.jsonandyarn.lockhave been properly updatedOther: